Skip to content

Vga mode #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 5 commits into from
Closed

Vga mode #102

wants to merge 5 commits into from

Conversation

RKennedy9064
Copy link
Member

Here's the initial PR as mentioned in #99. I've commented the public facing methods so far, but wanted to wait for the private methods until this initial PR is reviewed/cleaned up since it's so big. My first goal was to get everything working, so I'm sure there are enhancements that can be made and things that can be done better.

I also had to add 2 dependencies that I used to help implement everything. 1 is spinning_top and the other is conquer_once. I'm okay with using different methods if you don't want to add those as dependencies to bootloader. I'm also not sure if I added them correctly to cargo.toml, I just followed what was already there.

One last thing to note is that addresses 0xa0000 -> 0xbffff will need to be mapped in order for these features to work correctly. I manually mapped them in my kernel for testing, but I wasn't sure where you wanted it mapped here so I left that part out for now. Let me know what you think when you get a chance.

@RKennedy9064 RKennedy9064 mentioned this pull request Mar 11, 2020
@phil-opp
Copy link
Member

Thanks a lot for this PR!

I only took a quick look, but it seems like this does not really need to be part of the bootloader itself since it interacts with the VGA hardware through normal port I/O (in contrast to requiring BIOS calls). So I would propose that we create a separate crate for this. If you like, we could create it under the rust-osdev organization, with you getting admin privileges on this repo of course.

A separate crate has the advantage that it keeps the bootloader small, so that people that don't use the VGA functionality do not need to compile it into their projects, resulting in shorter compile times. It also allows people that use other bootloaders to still use the VGA crate, so that your code is useful to more people.

What do you think?

@RKennedy9064
Copy link
Member Author

I'm okay with creating a separate crate. I wasn't 100% bootloader was the right spot either. Just let me know what I need to do to help get it set up. Would it make sense to at least add the address mappings for 0xa0000 -> 0xbffff like you did for 0xb8000 to bootloader?

@phil-opp
Copy link
Member

I sent you an invite to the rust-osdev organization. After accepting, you should be able to create a new repository under the organziation.

Would it make sense to at least add the address mappings for 0xa0000 -> 0xbffff like you did for 0xb8000 to bootloader?

I'm not sure whether it's a good idea to add these mappings by default because not everyone will want them. But we could add an optional feature or a configuration option for this.

@phil-opp
Copy link
Member

Let me know if you need any help setting up the repository!

@RKennedy9064
Copy link
Member Author

Thanks for the access. I got the repo setup and am working on configuring a workflow for automatic testing. I'd be fine with a configuration option for those mappings. It would be nice to be able to turn them on so I don't have to map it all in my tests. Is that something I can add and open a PR for? If so, where do you currently handle mapping 0xb8000 for the 80x25 frame buffer?

@phil-opp
Copy link
Member

Thanks for the access. I got the repo setup and am working on configuring a workflow for automatic testing.

Great!

I'd be fine with a configuration option for those mappings. It would be nice to be able to turn them on so I don't have to map it all in my tests. Is that something I can add and open a PR for? If so, where do you currently handle mapping 0xb8000 for the 80x25 frame buffer?

Currently as part of the assembly code for the video modes: https://github.com/rust-osdev/bootloader/search?q=vga_map_frame_buffer&type=. The reason for this is that the bootloader itself already prints to the VGA buffer in some cases.

Looking at this code, I think it might make more sense to just identity map all pages in 0xa0000 – 0xbffff as you proposed, at least for now. We could still add a config option or a cargo feature for this later so that it is possible to disable the mapping if desired (or maybe additionally identity-map other pages).

@RKennedy9064
Copy link
Member Author

Is that something you want me to add, or do you want to? Also if you have a some spare time, any chance you can look at the github action for https://github.com/rust-osdev/vga? I tried setting something similar up to how x86_64 works and everything works except for the cargo xtest step, which is weird because I can run it locally. I haven't really worked with github actions much.

@phil-opp
Copy link
Member

I'd happy to merge a PR for this if you have time!

Also if you have a some spare time, any chance you can look at the github action for https://github.com/rust-osdev/vga? I tried setting something similar up to how x86_64 works and everything works except for the cargo xtest step

I think the problem was that the project config file was named cargo.toml and not Cargo.toml. I opened rust-osdev/vga#1 to correct this.

@RKennedy9064
Copy link
Member Author

This is now part of https://github.com/rust-osdev/vga

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants